-
-
Notifications
You must be signed in to change notification settings - Fork 32
refactor: update all relative export paths to use ts extension #541
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: update all relative export paths to use ts extension #541
Conversation
tbh, I don't see the need to use tsup/tsdown. can we just use tsc:
|
Honestly, that's fair, since we're not bundling. The only thing is that those could be faster, but I'd have to do a comparison (if that's important to you). |
Are |
The two main motivators i see are:
|
bd53ff1
to
bdc07e0
Compare
"lint:js": "eslint --cache --ignore-pattern \"**/*.md\"", | ||
"lint:js-docs": "eslint --no-inline-config \"**/*.md\"", | ||
"lint:js": "eslint --cache --ignore-pattern \"**/*.md\" --flag unstable_native_nodejs_ts_config", | ||
"lint:js-docs": "eslint --no-inline-config \"**/*.md\" --flag unstable_native_nodejs_ts_config", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get rid of jiti
if we're using this flag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We wouldn't need it for the eslint
run anymore. But it’s still being used by eslint-remote-tester
to parse that config. And, unfortunately, they don't have an option to use node's built-in, like eslint does.
Just want to verify the output would still be compatible with the minimum Node versions we support? ( |
Yes, the output tsdown generates still has .js extensions on the imports, and this wouldn't change the compatability of what it emits. I can prepare a before / after diff to share? Unfortunately, adding those versions to the test matrix wouldn't really be an adequate test, since the tests are run on source rather than the build output. But i like that idea. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
dts: true, | ||
entry: ['lib/**/*.ts'], | ||
format: ['esm'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any behavior changes from this tsup/tsdown conversion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's meant to be a drop in replacement, just faster. If we were able to set isolatedModules
to true in the tsconfig, it would be even faster. But the way the plugin is setup, we can't really do that, and still get rich type information. So, yeah, just swapping one for the other, won't change anything. I was able to remove some of those settings because tsdown will infer them from the package.json.
Coming from #540. This change shifts all relative imports to use
.ts
extensions, and enablesrewriteRelativeImportExtensions
in the tsconfig. Unfotunatelytsup
has a bug when using this setting (egoist/tsup#1271), so I switched bundlers totsdown
the Vite community's newtsup
alternative (which does support this).I also updated the lint CI step to force using the latest LTS (22.18.0) which comes with native type stripping. I also added the eslint flag that allows eslint to use native type stripping.